Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Favorites section header from NTP #3381

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

dus7
Copy link
Contributor

@dus7 dus7 commented Sep 23, 2024

Task/Issue URL: https://app.asana.com/0/0/1208244619690577/f
Tech Design URL:
CC:

Description:

Removes Favorites header when no favorites present. This allowed to removed whole EmptyStateView. Leftover function was incorporated into FavoritesViewModel, allowing to remove the generic type from NewTabPageView.

Steps to test this PR:

  1. Open NTP without having any favorites, make sure there's an add button and placeholders are visible.
  2. Rotate to landscape. Verify whole row is filled up with placeholders.
  3. Pixel should be fired when any placeholder is tapped.

Definition of Done (Internal Only):

Orientation Testing:

  • Portrait
  • Landscape

Internal references:

Software Engineering Expectations
Technical Design Template

Copy link

github-actions bot commented Sep 23, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against d8337fa

@dus7 dus7 force-pushed the mariusz/ntp-remove-favorites-header branch 2 times, most recently from 35b9e73 to e5f9c63 Compare September 23, 2024 13:40
@dus7 dus7 requested a review from brindy September 23, 2024 13:59
@dus7 dus7 marked this pull request as ready for review September 23, 2024 14:00
@dus7 dus7 force-pushed the mariusz/ntp-remove-favorites-header branch from 6404f16 to 390354c Compare September 23, 2024 14:01
@dus7 dus7 marked this pull request as draft September 23, 2024 14:02
Comment on lines +95 to +109
func prefixedFavorites(for columnsCount: Int) -> FavoritesSlice {
let hasFavorites = allFavorites.contains(where: \.isFavorite)
let maxCollapsedItemsCount = hasFavorites ? columnsCount * 2 : columnsCount
let isCollapsible = allFavorites.count > maxCollapsedItemsCount

var favorites = isCollapsed ? Array(allFavorites.prefix(maxCollapsedItemsCount)) : allFavorites

if !hasFavorites {
for _ in favorites.count ..< maxCollapsedItemsCount {
favorites.append(.placeholder(UUID().uuidString))
}
}

return .init(items: favorites, isCollapsible: isCollapsible)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real changes are here. (62a3c66)

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dus7 dus7 marked this pull request as ready for review September 25, 2024 10:40
@dus7 dus7 merged commit d7731d4 into main Sep 25, 2024
16 checks passed
@dus7 dus7 deleted the mariusz/ntp-remove-favorites-header branch September 25, 2024 10:42
samsymons added a commit that referenced this pull request Sep 28, 2024
# By Daniel Bernal (12) and others
# Via Daniel Bernal (5) and others
* main: (46 commits)
  Release 7.139.0-4 (#3411)
  Onboarding highlights experiment updates (#3406)
  fix suggestions performance (#3405)
  For third party requests differentiate if they are affiliated with first party (#3386)
  Bump BSK to pull in C-S-S 6.19.0 (#3396)
  Release 7.139.0-3 (#3399)
  Onboarding Highlights Ship Review  (#3380)
  add assertions for tabs in suggestions (#3394)
  Release 7.139.0-2 (#3398)
  Bump BSK to Include C.S.S 6.17 (#3397)
  Bump BSK which includes C.S.S 6.17 (#3395)
  Rever BSK branch
  Revert "Bump C.S.S"
  Bump C.S.S
  translations for Switch to Tab (#3391)
  Remove Favorites section header from NTP (#3381)
  Release 7.139.0-1 (#3389)
  Add origin to /apps URL (#3378)
  chery pick returning user fix
  Release 7.138.1-0 (#3388)
  ...

# Conflicts:
#	DuckDuckGo/AppDependencyProvider.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants